feat(DepositContract): Introduce operator pubkey mapping #2089
Conversation
WalkthroughThe changes include enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DepositContract
participant IDepositContract
User->>DepositContract: deposit(pubkey, credentials, amount, signature, operator)
DepositContract->>IDepositContract: validate deposit
IDepositContract-->>DepositContract: validation result
DepositContract->>DepositContract: register operator
DepositContract->>User: emit OperatorUpdated
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2089 +/- ##
===========================================
+ Coverage 23.50% 50.87% +27.36%
===========================================
Files 356 4 -352
Lines 16054 57 -15997
Branches 12 16 +4
===========================================
- Hits 3774 29 -3745
+ Misses 12115 28 -12087
+ Partials 165 0 -165
|
| @@ -94,6 +113,31 @@ abstract contract DepositContract is IDepositContract, ERC165 { | |||
| } | |||
| } | |||
|
|
|||
| function updateOperator( | |||
There was a problem hiding this comment.
@neverDefined are we okay with one step operator change process?
There was a problem hiding this comment.
two step would be good, on updates only though
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
contracts/test/staking/PermissionedDepositContract.sol (1)
Line range hint
1-70: Overall impact is positive. Consider updating documentation.The changes to the
PermissionedDepositContractsuccessfully integrate the newoperatorfunctionality from the parentDepositContractwhile maintaining the existing permission system. The contract's core functionality and inheritance structure remain intact.Consider updating the contract's documentation (e.g., the
@noticecomment at the top) to mention the new operator functionality. This will help developers understand the full capabilities of the contract at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- contracts/src/staking/DepositContract.sol (5 hunks)
- contracts/src/staking/IDepositContract.sol (3 hunks)
- contracts/test/staking/DepositContract.t.sol (12 hunks)
- contracts/test/staking/PermissionedDepositContract.sol (2 hunks)
- mod/geth-primitives/pkg/deposit/contract.abigen.go (5 hunks)
🧰 Additional context used
🔇 Additional comments (13)
contracts/src/staking/IDepositContract.sol (3)
28-36: LGTM! The addition ofOperatorUpdatedevent is well-implemented.The
OperatorUpdatedevent enhances operator management by providing transparency when an operator is updated. The event parameters are appropriately defined and the use ofindexedonpubkeyfacilitates efficient filtering.
60-68: LGTM! Custom errors improve error handling clarity.Introducing custom errors like
ZeroOperatorOnFirstDeposit(),NotCurrentOperator(), andZeroAddress()enhances the contract's error handling by providing more specific and gas-efficient revert messages.
73-83: LGTM! ThegetOperatorview function adds valuable functionality.The
getOperatorfunction allows users to retrieve the operator address associated with a givenpubkey. The implementation aligns with the interface design, and the documentation is clear and precise.contracts/src/staking/DepositContract.sol (3)
41-43: MappingoperatorByPubKeycorrectly associates public keys with operator addressesThe introduction of
operatorByPubKeymapping effectively links each public key to its corresponding operator address, which is essential for managing operator assignments.
59-62: FunctiongetOperatorcorrectly retrieves operator addressesThe
getOperatorfunction properly exposes the operator address associated with a given public key, allowing external contracts and users to query this information as needed.
92-101: Operator is correctly set during the first depositThe logic to assign the operator on the first deposit is sound. By checking if the public key is unregistered and ensuring a non-zero operator address, it prevents misuse and maintains data integrity.
contracts/test/staking/DepositContract.t.sol (3)
232-244: Confirm error handling intest_UpdateOperatorNotCurrentOperator.The test correctly checks for reverts when the caller is not the current operator. Ensure that the
NotCurrentOperatorerror is appropriately emitted when expected.
246-251: Validate zero address check intest_UpdateOperatorZeroAddress.The test appropriately expects a revert when attempting to set the operator to the zero address, ensuring robustness against invalid inputs.
253-262:test_UpdateOperatorcorrectly verifies operator update functionality.The test successfully confirms that the operator can be updated and the corresponding event is emitted with correct parameters.
mod/geth-primitives/pkg/deposit/contract.abigen.go (4)
34-35: ABI and Binary Updated to Include New FunctionsThe ABI and binary have been updated to include the new functions and events, reflecting the changes made to the contract interface.
267-296: Addition ofGetOperatorFunctionThe
GetOperatorfunction has been correctly added to the contract bindings to retrieve the operator associated with a given public key.
538-557: Addition ofUpdateOperatorFunctionThe
UpdateOperatorfunction has been successfully added to allow updates to the operator associated with a public key.
454-472: UpdateDepositFunction Calls to Include New Operator ParameterThe
Depositfunction signature has been updated to include theoperatorparameter. Ensure that all invocations of this function across the codebase have been updated to pass the new parameter to prevent any compilation or runtime errors.Run the following script to find all calls to
Depositand verify they include the new parameter:
| bytes calldata signature, | ||
| address operator |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider enhancing error handling.
The changes to the deposit function are correct and align with the modifications in the parent DepositContract. The new operator parameter is properly included in the function signature and passed to the superclass's deposit function.
Consider adding a check for the operator address to ensure it's not the zero address. This can prevent potential issues with invalid operators. You could add this check right after the existing authorization check:
if (operator == address(0)) revert InvalidOperator();Don't forget to define the InvalidOperator error at the top of the contract if you decide to implement this suggestion.
Also applies to: 53-55
| * @param operator is the address of the operator. | ||
| * @dev emits the Deposit event upon successful deposit. | ||
| */ | ||
| function deposit( | ||
| bytes calldata pubkey, | ||
| bytes calldata credentials, | ||
| uint64 amount, | ||
| bytes calldata signature | ||
| bytes calldata signature, | ||
| address operator |
There was a problem hiding this comment.
💡 Codebase verification
Some deposit calls are missing the new operator parameter. Please update all implementations and invocations to include it.
🔗 Analysis chain
Verify all implementations and calls to deposit include the new operator parameter.
Adding the operator parameter to the deposit function in IDepositContract is a breaking change. Ensure that all contracts implementing this interface and any external calls have been updated to match the new function signature to prevent integration issues.
Run the following script to identify implementations and calls that may need updating:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations and invocations of `deposit` that may lack the new `operator` parameter.
# Search for `deposit` function definitions excluding the interface file.
rg --glob '!contracts/src/staking/IDepositContract.sol' -t solidity -A 3 'function deposit\('
# Search for invocations of the `deposit` function in Solidity files.
rg -t solidity -A 1 '\.deposit\('
Length of output: 197
Script:
#!/bin/bash
# Description: Find all implementations and invocations of `deposit` that may lack the new `operator` parameter.
# Search for `deposit` function definitions excluding the interface file.
rg --glob '!contracts/src/staking/IDepositContract.sol' -e sol -A 3 'function deposit\('
# Search for invocations of the `deposit` function in Solidity files.
rg -e sol -A 1 '\.deposit\('
Length of output: 232
Script:
#!/bin/bash
# Description: Find all implementations and invocations of `deposit` in Solidity files.
# Define a custom file type for Solidity if not already recognized
rg --type-add 'solidity:*.sol' --type solidity -A 3 'function\s+deposit\s*\(' --glob '!contracts/src/staking/IDepositContract.sol'
# Search for invocations of the `deposit` function in Solidity files
rg --type-add 'solidity:*.sol' --type solidity -A 1 '\.deposit\s*\('
Length of output: 2188
| bytes calldata signature, | ||
| address operator |
There was a problem hiding this comment.
💡 Codebase verification
Multiple deposit calls missing the operator parameter. Please update all invocations to include the new parameter.
🔗 Analysis chain
Verify all calls to deposit include the new operator parameter
With the addition of the operator parameter to the deposit function, ensure that all existing calls to this function throughout the codebase have been updated accordingly to prevent any breaking changes.
Run the following script to locate all invocations of deposit and check for compliance:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `deposit` and display their usage.
# Expected Result: All calls should include the new `operator` parameter.
rg -A 2 "deposit\(" --glob "contracts/**/*.sol"
Length of output: 3228
Script:
#!/bin/bash
# Description: Find all calls to `deposit` and count the number of arguments passed.
# Expected Result: All calls should have the expected number of parameters (including `operator`).
rg -n 'deposit\s*\(' --glob 'contracts/**/*.sol' | while read -r line; do
file=$(echo "$line" | cut -d':' -f1)
lineno=$(echo "$line" | cut -d':' -f2)
call=$(echo "$line" | cut -d':' -f3-)
# Extract the arguments within the parentheses
args=$(echo "$call" | sed -n 's/.*deposit\s*(\([^)]*\)).*/\1/p')
# Count the number of arguments by splitting on commas not within parentheses
arg_count=$(echo "$args" | awk -F',' '{print NF}')
echo "$file:$lineno: $arg_count arguments"
done
Length of output: 9496
| _create96Byte(), | ||
| operator |
There was a problem hiding this comment.
Use pubKey parameter in deposit call for proper fuzz testing.
In testFuzz_DepositsWrongPubKey, the deposit function is called with a hardcoded bytes("wrong_public_key") instead of the pubKey parameter. To effectively fuzz test with various invalid public keys, use the pubKey parameter in the deposit call.
Apply this diff to fix the issue:
depositContract.deposit(
- bytes("wrong_public_key"),
+ pubKey,
STAKING_CREDENTIALS,
32e9,
_create96Byte(),
operator
);Committable suggestion was skipped due to low confidence.
| VALIDATOR_PUBKEY, | ||
| bytes("wrong_credentials"), | ||
| 32e9, | ||
| _create96Byte(), | ||
| operator |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use consistent invalid credentials in test_DepositWrongCredentials.
In test_DepositWrongCredentials, consider using a consistent placeholder for invalid credentials similar to other tests. This enhances readability and consistency across tests.
Apply this diff for consistency:
VALIDATOR_PUBKEY,
- bytes("wrong_credentials"),
+ INVALID_CREDENTIALS,
32e9,
_create96Byte(),
operatorWhere you define INVALID_CREDENTIALS at the beginning of the contract:
+ bytes internal constant INVALID_CREDENTIALS = bytes("wrong_credentials");Committable suggestion was skipped due to low confidence.
| Pubkey common.Hash | ||
| NewOperator common.Address | ||
| PreviousOperator common.Address | ||
| Raw types.Log // Blockchain specific contextual infos | ||
| } |
There was a problem hiding this comment.
Incorrect Type for Pubkey in OperatorUpdated Event
The Pubkey field in the BeaconDepositContractOperatorUpdated struct is defined as common.Hash, but the original Solidity event defines pubkey as bytes, which may exceed 32 bytes. Using common.Hash may lead to truncation of pubkey if it is longer than 32 bytes.
To accurately capture the pubkey, consider changing the type to []byte:
-type BeaconDepositContractOperatorUpdated struct {
- Pubkey common.Hash
+type BeaconDepositContractOperatorUpdated struct {
+ Pubkey []byte
NewOperator common.Address
PreviousOperator common.Address
Raw types.Log // Blockchain specific contextual infos
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Pubkey common.Hash | |
| NewOperator common.Address | |
| PreviousOperator common.Address | |
| Raw types.Log // Blockchain specific contextual infos | |
| } | |
| type BeaconDepositContractOperatorUpdated struct { | |
| Pubkey []byte | |
| NewOperator common.Address | |
| PreviousOperator common.Address | |
| Raw types.Log // Blockchain specific contextual infos | |
| } |
|
Closing this, will moving whole Deposit contract logic to monorepo |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation